Fix breadcrumb error on GitHub detail page#2084
Conversation
🦋 Changeset detectedLatest commit: 1640598 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@claude --review and add a changeset based on changes in this PR |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This PR cleanly fixes the breadcrumb error on the GitHub detail page by adding a github fetcher and makes several well-executed consistency improvements:
- ✅ GitHub breadcrumb fix: Adds
fetchWorkAppGitHubInstallationDetailto resolve installation account names - ✅ Type safety: Adopts
PageProps<>generic over manual interface definitions - ✅ React.cache() placement: Correctly moves caching to the API client layer (
agent-full-client.ts) - ✅ Server component conversion: Appropriately converts
work-apps/page.tsxandwork-apps/slack/page.tsxto async server components - ✅ Sentry integration: Adds error capture for unknown breadcrumb segments
- ✅ Biome config: Reasonable exclusion of JSON snapshot files
💭 Consider (1) 💭
💭 1) multi-file Gradual PageProps migration leaves split-world
Issue: The PR correctly converts several pages from manual { params: Promise<...> } interfaces to the PageProps<> generic type. However, some sibling files in similar paths still use the manual pattern.
Why: Creates a temporary split-world situation where both patterns coexist in [tenantId]/stats/ and other directories. Not blocking since the PR moves in the right direction.
Fix: Consider migrating remaining pages using manual interfaces to PageProps<> in a follow-up PR, or document that both patterns are acceptable during transition.
Refs:
- stats/ai-calls/page.tsx — uses manual interface
- projects/page.tsx — uses PageProps (established pattern)
✅ APPROVE
Summary: Clean, well-executed bug fix with appropriate consistency improvements. All changes align with established codebase patterns. The React.cache() relocation, PageProps<> adoption, and server component conversions are all correct. Ship it! 🚀
Discarded (6)
| Location | Issue | Reason Discarded |
|---|---|---|
agent-full-client.ts:112 |
React.cache() added to getFullAgent | Confirmed as correct pattern — matches fetchProject, fetchSkill, etc. |
agent-full.ts:161 |
Cache wrapper removed from server action | Valid cleanup — cache moved to API client layer |
@breadcrumbs/[...slug]/page.tsx:6 |
API client used instead of action wrapper | Correct — aligns with other fetchers in same file |
work-apps/page.tsx |
Converted to server component | Correct — no client hooks used |
work-apps/slack/page.tsx |
Converted to server component | Correct — no client hooks used |
github pages |
Retained as client components | Correct — uses useState/useEffect hooks |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
5 | 0 | 1 | 0 | 0 | 0 | 4 |
pr-review-frontend |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
| Total | 8 | 0 | 1 | 0 | 0 | 0 | 7 |
Co-authored-by: Dimitri POSTOLOV <dimaMachina@users.noreply.github.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
🔴❗ Critical (0) ❗🔴
No critical issues identified.
🟠⚠️ Major (0) 🟠⚠️
No major issues identified.
🟡 Minor (0) 🟡
No minor issues identified.
💭 Consider (1) 💭
💭 1) agent-full-client.ts / agent-full.ts Two APIs for same agent data
Issue: The breadcrumbs now use getFullAgent (client API with cache()) while other pages still use getFullAgentAction (server action). Both fetch the same data but have different patterns.
Why: This is intentional for breadcrumbs to benefit from cache() deduplication (called twice for agents and webhooks fetchers), but could cause confusion about which to use when.
Fix: Consider adding a code comment in agent-full-client.ts explaining when to prefer this API vs the server action, or document this pattern for future contributors.
Refs:
- agent-full-client.ts:112 — getFullAgent with cache()
- agents/[agentId]/page.tsx — uses getFullAgentAction
✅ APPROVE
Summary: This is a clean bug fix that correctly adds a GitHub breadcrumb fetcher and migrates to the cached client API for better request deduplication. The server-to-client component conversions follow valid Next.js patterns, and the Biome config change appropriately excludes snapshot files. Ship it! 🚀
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
work-apps/page.tsx, work-apps/slack/page.tsx |
RSC boundary concern with SlackProvider | Valid Next.js pattern: server components can render client component boundaries. The reviewer's own analysis noted loading states handle this correctly. |
agent-full-client.ts |
Missing 'use server' directive |
Intentional - this is a client API module, not a server action. The cache() function works in server components regardless. |
breadcrumbs/page.tsx:52 |
Less explicit error handling | Reviewer noted this is "a reasonable simplification" and the try-catch block handles thrown errors correctly. |
biome.jsonc |
Snapshot pattern only covers JSON | Intentional and correct - the only snapshot file is openapi.json. |
| Multiple files | Positive pattern confirmations | INFO-level findings confirming correct patterns, not issues. |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
4 | 0 | 1 | 0 | 0 | 0 | 3 |
pr-review-frontend |
5 | 0 | 0 | 0 | 0 | 0 | 5 |
pr-review-devops |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
| Total | 12 | 0 | 1 | 0 | 0 | 0 | 11 |
* upd * upd * upd * fix lint * Add changeset for breadcrumb fix Co-authored-by: Dimitri POSTOLOV <dimaMachina@users.noreply.github.com> --------- Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Dimitri POSTOLOV <dimaMachina@users.noreply.github.com>
1 participant
No description provided.